Skip to content

fix(dependencies): @types/glob latest versions do not work with Minimatch. Removed refer… #33231

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

fix(dependencies): @types/glob latest versions do not work with Minimatch. Removed refer… #33231

wants to merge 1 commit into from

Conversation

@QuantumNeuralCoder
Copy link
Contributor

@QuantumNeuralCoder QuantumNeuralCoder commented Jan 30, 2025 •
edited
Loading

@types/glob latest versions do not work with Minimatch.
So removed references to @types/glob from CDK tools. Added dependency glob. This was as per recommendation from the author.
Fixed Promise code to use async api from new dependency glob in cdk-copy.

Issue # (if applicable)

None

Closes #
Build issues for #33168

Reason for this change

Build is broken since @types/glob latest versions do not work with Minimatch latest version due to breaking changes. The author of @types/glob has rewritten the module and released in Glob.
Updating the references and removing the old dependency caused an error in its usage in

cdk-copy 
``` tool.
The new modul provides an async interface. Refactored cdk-copy to use the async interface and made minor improvements.

### Description of changes

What code changes did you make? 
See above.
Have you made any important design decisions?
No
What AWS use cases does this change enable? To enable the use cases, which AWS service features are utilized?
No

### Describe any new or updated permissions being added

What new or updated IAM permissions are needed to support the changes being introduced ? 
None


### Description of how you validated changes

Build passes. 

### Checklist
- [ x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

@types/glob latest versions do not work with Minimatch. Removed refer…
818397d 
…ences to @types/glob from CDK tools. Added dependency glob. Fixed Promise code to use async api from new dependency glob in cdk-copy.
@QuantumNeuralCoder QuantumNeuralCoder requested a review from a team as a code owner January 30, 2025 02:04
@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels Jan 30, 2025
@aws-cdk-automation aws-cdk-automation requested a review from a team January 30, 2025 02:04
@aws-cdk-automation
Copy link
Collaborator

aws-cdk-automation commented Jan 30, 2025

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 818397d
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation
aws-cdk-automation requested changes Jan 30, 2025
View reviewed changes
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.

@codecov
Copy link

codecov bot commented Jan 30, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.79%. Comparing base (6fa1d05) to head (818397d).
Report is 4 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #33231   +/-   ##
=======================================
  Coverage   80.79%   80.79%           
=======================================
  Files         232      232           
  Lines       14106    14106           
  Branches     2452     2452           
=======================================
  Hits        11397    11397           
  Misses       2429     2429           
  Partials      280      280
Flag Coverage Δ
suite.unit 80.79% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
packages/aws-cdk 79.53% <ø> (ø)
packages/aws-cdk-lib/core 82.17% <ø> (ø)

mrgrain
mrgrain requested changes Jan 30, 2025
View reviewed changes
Copy link
Contributor

@mrgrain mrgrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't do that right because the updated dependency now requires Node >= 20.

There's a discussion to be had about that node versions we want to support though. @iliapolo was having some thoughts and this recently as well.

@mrgrain mrgrain added the pr/do-not-merge This PR should not be merged at this time. label Jan 30, 2025
@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 30, 2025

Echoing what Momo is saying. Unless there is a very good reason to upgrade (i.e., CVE, missing feature), the solution to all of these upgrade woes is to... not do it!

So let's not upgrade minimatch, then we also don't have to upgrade glob, and we also don't have to upgrade @types/glob.

@QuantumNeuralCoder
Copy link
Contributor Author

QuantumNeuralCoder commented Jan 30, 2025

Ack. Do we have a plan to handle these? #33168

@mrgrain
Copy link
Contributor

mrgrain commented Jan 30, 2025

Ack. Do we have a plan to handle these? #33168

Interesting. Other than the build error, there's nothing indicating how the upgrade dependencies PR is causing that problem.

The general approach would be to upgrade all the dependency, except the ones that are causing an issue. And to prevent this from happening again tomorrow you would exclude the offending dependency form upgrades. Alternatively overrides maybe used to achieve a similar result.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 30, 2025

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 30, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mrgrain mrgrain mrgrain requested changes

@aws-cdk-automation aws-cdk-automation aws-cdk-automation requested changes

Assignees

No one assigned

Labels

beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 pr/do-not-merge This PR should not be merged at this time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@QuantumNeuralCoder @aws-cdk-automation @rix0rrr @mrgrain